Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pymethods: seq methods from mapping methods #2065

Merged
merged 1 commit into from
Feb 5, 2022

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Dec 20, 2021

Cut out from #2060

This is the part of that PR which changes __getitem__, __setitem__, and __delitem__ to implement the sequence slots as well as the mapping slots.

I think it's reasonably clear that this design makes sense, because it matches pure-Python behavior. It defers how to solve "pure-sequence" and "pure-mapping" classes to #2060 - which might either be the __seqlen__ (etc.) methods or attributes like #[pyo3(sequence)] and #[pyo3(mapping)].

Note that I ended up making the decision to not implement sq_length from __len__, because this way Python won't automatically try to convert negative indices to positive ones by adding the sequence length. This behaviour is inconsistent, because it only applies to calls to __getitem__ etc from the sequence slot (not the mapping slot). We can't reverse it on the PyO3 side either, because if we get a positive index to the sequence slot we don't know whether the original index was actually a negative one but had the length added. From searching the CPython source I don't see any downsides to not implementing sq_length except that PySequence::len() will fail for pyclasses.

I'm tempted to submit a PR to upstream CPython to see if there can be a different way to opt out of this length-correcting behaviour so that we can implement sq_length too.

@davidhewitt davidhewitt mentioned this pull request Dec 27, 2021
10 tasks
src/class/impl_.rs Outdated Show resolved Hide resolved
src/class/impl_.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Does anyone have any thoughts on whether proceeding with this PR is the right approach? I think it's still the best idea I could come up with for the moment, and it's reasonably simple. It gets us enough progress that we can deprecate pyproto, and we can invite users who need cases this simplified design can't solve to help us hash out further refinements.

If I don't hear anything, I'll tidy up, rebase and merge this in about a week's time.

@aganders3
Copy link
Contributor

For what it's worth this makes sense to me given that (as you mention) it mimics the pure-Python behavior.

I also like the proposed #[pyo3(sequence)] and #[pyo3(mapping)] attributes for the "pure" sequence and mapping types.

@davidhewitt
Copy link
Member Author

davidhewitt commented Jan 19, 2022

👍I have since also come to the conclusion that #[pyo3(sequence)] and #[pyo3(mapping)] would set the underlying type flags, which would be necesary for Python match support and may also interact with #2072.

I'll introduce those in a follow-up PR (perhaps not until 0.17).

@davidhewitt davidhewitt force-pushed the protos-seq-mapping-fallbacks-2 branch 2 times, most recently from 8d686b1 to a8f2f70 Compare February 5, 2022 15:53
@davidhewitt
Copy link
Member Author

Took me three weeks to find a chance to finish this off! Merging now 😄

@davidhewitt davidhewitt merged commit 6db7dd7 into PyO3:main Feb 5, 2022
@davidhewitt davidhewitt deleted the protos-seq-mapping-fallbacks-2 branch February 5, 2022 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants